-
Notifications
You must be signed in to change notification settings - Fork 13k
chore: store media call signals on mongo for 3 days #37069
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a new MediaCallChannelLogs model and typings, registers it in the Meteor server, and integrates non-blocking logging of media signaling (send/recv) in CallSignalProcessor. Defines TTL and composite indexes for the new collection. Exposes new interfaces and model types via core/model typings and models package indices. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UA as UserActorSignalProcessor
participant MCP as MediaCallChannelLogs Model
participant HS as Handler/Server
rect rgba(220,240,255,0.4)
note over UA: processSignal (incoming)
UA->>MCP: insert({ callId, channelId, direction: "recv", ts, content })
note over MCP: Non-blocking (errors ignored)
UA->>HS: handle incoming signal
end
rect rgba(220,255,220,0.4)
note over UA: sendSignal (outgoing)
UA->>MCP: insert({ callId, channelId, direction: "send", ts, content })
note over MCP: Non-blocking (errors ignored)
UA->>HS: forward signal to server
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37069 +/- ##
===========================================
+ Coverage 67.79% 67.82% +0.03%
===========================================
Files 3345 3343 -2
Lines 114470 114350 -120
Branches 20716 20754 +38
===========================================
- Hits 77605 77563 -42
+ Misses 34178 34104 -74
+ Partials 2687 2683 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/models/src/index.ts (1)
234-267: Consider registering the model for service contexts too (optional).If any non-Meteor service needs to read/write channel logs, add a registration in registerServiceModels; otherwise ignore.
Apply this diff if needed:
+import { MediaCallChannelLogsRaw } from './modelClasses'; ... export function registerServiceModels(db: Db, trash?: Collection<RocketChatRecordDeleted<any>>): void { ... + registerModel('IMediaCallChannelLogsModel', () => new MediaCallChannelLogsRaw(db)); }packages/core-typings/src/mediaCalls/IMediaCallChannelLog.ts (1)
3-13: Interface shape looks correct.Minimal fields with TTL on ts at the model layer. Consider typing content more narrowly later to improve safety.
packages/models/src/models/MediaCallChannelLogs.ts (3)
12-21: Add a compound index including ts to support time-ordered queriesLikely access pattern: fetch recent events for a call/channel sorted by ts. Without ts in the index, sorting/range scans will be less efficient.
Apply this diff to add a supporting index (keep TTL on ts as a separate single-field index):
protected modelIndexes(): IndexDescription[] { return [ { key: { callId: 1, channelId: 1 }, unique: false, }, + { + key: { callId: 1, channelId: 1, ts: -1 }, + }, // Allow 3 days of events to be saved { key: { ts: 1 }, expireAfterSeconds: 3 * 24 * 60 * 60 }, ]; }
18-20: Name the TTL index and avoid magic numbers (use a constant)Adds stability across driver versions and eases future retention changes.
Apply this diff here:
- { key: { ts: 1 }, expireAfterSeconds: 3 * 24 * 60 * 60 }, + { key: { ts: 1 }, expireAfterSeconds: MEDIA_CALL_LOG_TTL_SECONDS, name: 'ttl_ts' },And add this at the top of the file (outside the selected lines):
const MEDIA_CALL_LOG_TTL_SECONDS = 3 * 24 * 60 * 60; // 3 days
15-17: Drop redundant unique: falseNon-unique is the default; removing it simplifies the spec.
Apply this diff:
- { - key: { callId: 1, channelId: 1 }, - unique: false, - }, + { + key: { callId: 1, channelId: 1 }, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
apps/meteor/server/models.ts(2 hunks)ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts(3 hunks)packages/core-typings/src/mediaCalls/IMediaCallChannelLog.ts(1 hunks)packages/core-typings/src/mediaCalls/index.ts(1 hunks)packages/model-typings/src/index.ts(1 hunks)packages/model-typings/src/models/IMediaCallChannelLogsModel.ts(1 hunks)packages/models/src/index.ts(2 hunks)packages/models/src/modelClasses.ts(1 hunks)packages/models/src/models/MediaCallChannelLogs.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/meteor/server/models.ts (1)
packages/models/src/models/MediaCallChannelLogs.ts (1)
MediaCallChannelLogsRaw(7-22)
ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (1)
packages/models/src/index.ts (1)
MediaCallChannelLogs(189-189)
packages/models/src/models/MediaCallChannelLogs.ts (2)
packages/core-typings/src/mediaCalls/IMediaCallChannelLog.ts (1)
IMediaCallChannelLog(3-13)packages/model-typings/src/models/IMediaCallChannelLogsModel.ts (1)
IMediaCallChannelLogsModel(5-5)
packages/model-typings/src/models/IMediaCallChannelLogsModel.ts (1)
packages/core-typings/src/mediaCalls/IMediaCallChannelLog.ts (1)
IMediaCallChannelLog(3-13)
packages/models/src/index.ts (2)
packages/core-services/src/index.ts (1)
proxify(131-131)packages/model-typings/src/models/IMediaCallChannelLogsModel.ts (1)
IMediaCallChannelLogsModel(5-5)
🔇 Additional comments (9)
packages/model-typings/src/models/IMediaCallChannelLogsModel.ts (1)
1-5: Typings addition looks good.Straightforward alias to IBaseModel with correct import types.
packages/models/src/index.ts (2)
95-97: Good addition to public typings import list.Keeps proxify typing aligned.
189-189: Proxified model export looks correct.Identifier matches the registration key and typing.
packages/model-typings/src/index.ts (1)
88-89: Barrel re-export LGTM.Exposes the new model typing publicly.
packages/core-typings/src/mediaCalls/index.ts (1)
3-3: Public re-export LGTM.Makes the log interface available to consumers.
packages/models/src/modelClasses.ts (1)
80-80: Model re-export LGTM.Surfaces MediaCallChannelLogsRaw consistently with other models.
apps/meteor/server/models.ts (2)
48-49: Import addition LGTM.Brings the new Raw model into the registry scope.
143-144: Model registration LGTM.Key matches proxify identifier; indexes will be created on startup.
packages/models/src/models/MediaCallChannelLogs.ts (1)
12-21: Verify index shapes against actual queries
No.find*or.sort({ ts:… })usages found; confirm whether queries filter bychannelIdor sort bytsand add a compound index{ channelId: 1, ts: -1 }if needed.
sampaiodiego
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'd need a very (VERY) good reason to do this. can you elaborate please? we should use regular logs for this purpose and not store on db
Proposed changes (including videos or screenshots)
With this change, all signals exchanged between rocket.chat client and server within the context of a voice call are going to be stored on a mongo collection for three days, so that they can be reviewed in case of unexpected issues.
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit